Skip to content

Conversation

sandangel
Copy link
Contributor

@sandangel sandangel commented Aug 17, 2025

Signed-off-by: San Nguyen vinhsannguyen91@gmail.com

Motivation and Context

The parse_qs function return dictionary, and will have to use .items() with for loop

How Has This Been Tested?

redirect_uri_base = "http://localhost:8000/callback?session_id=1234"

parsed_uri = urlparse(redirect_uri_base)

for k, vs in parse_qs(parsed_uri.query):
    print(f"{k}: {vs}")

This will throw error

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: too many values to unpack (expected 2)

Breaking Changes

No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Signed-off-by: San Nguyen <vinhsannguyen91@gmail.com>
@sandangel sandangel requested a review from a team as a code owner August 17, 2025 12:02
@sandangel sandangel requested a review from ochafik August 17, 2025 12:02
ihrpr
ihrpr previously approved these changes Aug 18, 2025
Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Please can tests be added to prevent regressions?

@ochafik ochafik removed their request for review August 19, 2025 15:01
Add comprehensive tests for the construct_redirect_uri function to ensure
proper handling of query parameters, including the bug fixed in PR modelcontextprotocol#1279
where parse_qs() required .items() to iterate over the dictionary.

The tests cover:
- Basic functionality with no existing parameters
- Preserving existing query parameters (regression test for modelcontextprotocol#1279)
- Multiple existing parameters
- None value filtering
- Empty parameter handling
- Duplicate parameter names
- Multi-valued parameters
- URL encoding

Reported-by: San Nguyen
Github-Issue: modelcontextprotocol#1279
@felixweinberger felixweinberger requested a review from ihrpr August 21, 2025 15:09
@felixweinberger
Copy link
Contributor

Thank you!

Please can tests be added to prevent regressions?

@ihrpr done!

@sandangel
Copy link
Contributor Author

Hi, @ihrpr , can we merge this PR?

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ihrpr ihrpr merged commit eaf7cf4 into modelcontextprotocol:main Aug 23, 2025
18 checks passed
@sandangel sandangel deleted the fix/error-too-many-values-to-unpack-expected-2 branch August 23, 2025 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants